Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix!: writeFileSafely関数をmove-file非依存にし、テストを追加 #2502

Merged
merged 3 commits into from
Jan 24, 2025

Conversation

Hiroshiba
Copy link
Member

@Hiroshiba Hiroshiba commented Jan 23, 2025

内容

writeFileSafely関数をmove-file非依存にし、テストを追加しました。

を解決します。
詳細はこちら

関連 Issue

fix: #2501

@Hiroshiba Hiroshiba requested a review from a team as a code owner January 23, 2025 17:00
@voicevox-preview-pages
Copy link

voicevox-preview-pages bot commented Jan 23, 2025

🚀 プレビュー用ページを作成しました 🚀

更新時点でのコミットハッシュ:ff477bf

@Hiroshiba
Copy link
Member Author

Hiroshiba commented Jan 23, 2025

@madosuki さん、あと @sabonerune さん、もしよかったらレビューいただけると心強いです!!
想定外の処理を思いついた、とか! 🙏

@sabonerune
Copy link
Contributor

sabonerune commented Jan 23, 2025

@Hiroshiba
非同期になっていないけれど多分大丈夫?


fs.renameSync()のドキュメントにはPOSIXのrename(2)のドキュメントを見ろしか書いていません。
rename(2)には移動先がディレクトリ名の場合ディレクトリの中に書き込まれるとあります。

move-filemkdirはそれを防ぐため?

一方コールバック版のfs.rename()は移動先にディレクトリがあるとエラーになるそうです。

試したところfs.renameSync()はどうやらfs.rename()と同じように移動先にディレクトリがあるとエラーになるので多分大丈夫です。

@madosuki
Copy link
Contributor

概ね問題無さそうです。
後はsaboneruneさんと同じく、非同期になってないのは気になります。

@Hiroshiba
Copy link
Member Author

お二人ともありがとうございます!!
非同期に関してはこのプルリクエストで諦めたので、タイトルを変更し忘れていました 🙇
変えておきます!!

@sabonerune
ディレクトリ名にかぶった時、なるほどです!
これちょっとテスト書いておいた方がいいですね・・・! 書き足しておきたいと思います!

@Hiroshiba Hiroshiba changed the title fix!: writeFileSafely関数を非同期に変更し、テストを追加 fix!: writeFileSafely関数をmove-file非依存にし、テストを追加 Jan 24, 2025
@Hiroshiba
Copy link
Member Author

問題ないと思うのでマージしたいと思います!

@Hiroshiba Hiroshiba enabled auto-merge January 24, 2025 13:43
@Hiroshiba Hiroshiba added this pull request to the merge queue Jan 24, 2025
Merged via the queue into VOICEVOX:main with commit 39d2948 Jan 24, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants